Add CLAUDE.md with guidance for AI coding assistants#29171
Add CLAUDE.md with guidance for AI coding assistants#29171raunaqmorarka merged 1 commit intotrinodb:masterfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3a8efdb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds a repository-level CLAUDE.md intended to be auto-loaded by AI coding assistants, documenting Trino-specific conventions and pointing to existing development documentation where appropriate.
Changes:
- Introduces
CLAUDE.mdwith guidance on code style, formatting/import conventions, configuration/session property conventions, and Maven build workflows. - Cross-references
.github/DEVELOPMENT.mdfor topics already covered elsewhere.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughA new contributor documentation file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CLAUDE.md (1)
17-17: Consider clarifying theformat()reference.The guidance mentions
format()with "(statically imported)" but doesn't specify the source. Consider adding the fully qualified name (e.g.,String.format()orio.trino.spi.TrinoException.format()) for first-time contributors who might be unsure what to import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 17, Clarify which format() is meant by updating the guidance to name the exact API used—e.g., replace the ambiguous "format() (statically imported)" with a fully qualified reference such as String.format() when referring to Java's standard method or io.trino.spi.TrinoException.format() if referring to the Trino helper, and mention that it should be statically imported only when the Trino-specific helper is intended; ensure the text briefly states which import to use so first-time contributors know which symbol to import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Line 17: Clarify which format() is meant by updating the guidance to name the
exact API used—e.g., replace the ambiguous "format() (statically imported)" with
a fully qualified reference such as String.format() when referring to Java's
standard method or io.trino.spi.TrinoException.format() if referring to the
Trino helper, and mention that it should be statically imported only when the
Trino-specific helper is intended; ensure the text briefly states which import
to use so first-time contributors know which symbol to import.
0c3a8ef to
0bdadc2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
eebd332 to
269785e
Compare
|
|
||
| Run `mcp__idea__reformat_file` after every Java edit — it applies the Airlift scheme imported into IntelliJ. Rules not covered by the formatter: | ||
|
|
||
| - No wildcard imports (e.g. `import io.trino.spi.*`) — checkstyle catches these on build; easier to avoid writing them. |
There was a problem hiding this comment.
Isn't that covered by airlift formatter?
There was a problem hiding this comment.
Tested this with mcp__idea__reformat_file on a file with import java.util.*; — the wildcard wasn't expanded. The MCP tool invokes "Reformat Code" only (no Optimize Imports, no Cleanup), so it doesn't pick up the Airlift scheme's wildcard-expansion behavior. Checkstyle still catches them at build time, but for Claude's first-draft output it helps to have the rule here. Keeping the bullet.
There was a problem hiding this comment.
The MCP tool invokes "Reformat Code" only (no Optimize Imports, no Cleanup), so it doesn't pick up the Airlift scheme's wildcard-expansion behavior.
We should teach Claude to optimize imports via intellij too then.
There was a problem hiding this comment.
Claude can only make intellij do whatever intellij MCP exposes
| Run `mcp__idea__reformat_file` after every Java edit — it applies the Airlift scheme imported into IntelliJ. Rules not covered by the formatter: | ||
|
|
||
| - No wildcard imports (e.g. `import io.trino.spi.*`) — checkstyle catches these on build; easier to avoid writing them. | ||
| - Braces required around single-statement `if` / `for` / `while` bodies — the formatter does not add missing braces. |
There was a problem hiding this comment.
my intellij somehow adds braces for if during reformat.
There was a problem hiding this comment.
Also tested — created a file with if (x > 0) System.out.println("hi");, ran mcp__idea__reformat_file, and the braces were not added. Same reason as the wildcard case: the MCP tool runs plain Reformat Code, not Reformat and Cleanup. Your IDE likely has "Reformat and Cleanup" bound or code cleanup enabled in the reformat dialog, but that doesn't carry over when Claude invokes the formatter through the MCP. Keeping the bullet.
There was a problem hiding this comment.
Then there are likely other things we would need to write here, if we cannot have Claude use Intellij's full reformatting power.
There was a problem hiding this comment.
I imagine Intellij MCP will get better over time, meanwhile it is fine to add more rules here if we need to.
Claude is pretty good about taking cues from existing code though, so we don't need to be exhaustive
|
@hashhar PTAL |
0e7d27e to
954065e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
954065e to
4410b21
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hashhar
left a comment
There was a problem hiding this comment.
Good start. Very closely matches what I have as well. I do have to often tell it to rebuild (using clean + -am or -amd) instead of manually trying to -pl it's way to a successful build. Sadly haven't managed to find the words to get it to stop doing that.
|
For testing I have unit test knowledge described in claude.md:
For product tests (click to see full content): Details# Product Tests
## Running
```bash
# List available suites and environments
./bin/ptl suite list
./bin/ptl env list
# Run a product test suite
./bin/ptl suite run --suite <suite-name>
# Run a product test group
./bin/ptl test run --environment <env-name> -- -g <group-name1>,<group-name2>
# Run a specific test
./bin/ptl test run --environment <env-name> -- -t <fully-qualified-method-name1>,<fully-qualified-method-name2>BuildingAfter changing product test code, rebuild both modules before running: mvnd install -pl :trino-product-tests,:trino-product-tests-launcher -DskipTests -Dair.check.skip-all=trueThe launcher copies the test jar and server-main into Docker containers, so they must be rebuilt too if required. Infrastructure
Framework
Test setup:
|
Top-level CLAUDE.md points at the authoritative code-style sources (.github/DEVELOPMENT.md, modernizer, checkstyle), documents the JetBrains MCP server usage, and lists the minimal Java formatting rules the IntelliJ reformatter does not auto-fix. Config-property conventions live as a path-scoped rule under .claude/rules/ so they activate automatically when Claude reads matching *Config.java files. .gitignore narrowed from the whole .claude/ directory to just settings.local.json and worktrees/, so project-level rules and future .claude/ content can be committed while per-user files stay ignored.
|
Thanks for sharing. The testing content fits the |
4410b21 to
470a8a9
Compare
Summary
CLAUDE.mddocumenting Trino-specific conventions for AI coding assistants (Claude Code and similar tools that auto-loadCLAUDE.mdas project context).@ConfigDescription,@LegacyConfig,@DefunctConfig), and Maven build rules..github/DEVELOPMENT.mdfor topics already documented there (Web UI build, release process, Vector API, IDE setup rationale) rather than duplicating them.Test plan